Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HHH-13788 Schema update try to recreate existing tables #3151

Merged
merged 3 commits into from
Jun 22, 2021
Merged

HHH-13788 Schema update try to recreate existing tables #3151

merged 3 commits into from
Jun 22, 2021

Conversation

dreab8
Copy link
Contributor

@dreab8 dreab8 commented Dec 17, 2019

@dreab8 dreab8 requested review from Sanne, sebersole and gsmet December 17, 2019 15:05
Sanne
Sanne requested changes Dec 18, 2019
View reviewed changes
...est/java/org/hibernate/test/schemaupdate/SchemaUpdateTestWithUseJdbcMetadataDeaultsTest.java Outdated
}

@After
public void tearsDown() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"tears" Down ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a typo

...est/java/org/hibernate/test/schemaupdate/SchemaUpdateTestWithUseJdbcMetadataDeaultsTest.java Outdated
.execute( EnumSet.of( TargetType.DATABASE, TargetType.SCRIPT ), metadata );

final String fileContent = new String( Files.readAllBytes( output.toPath() ) );
assertThat( "The update output file should be empty", fileContent, is( "" ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand this test. Why does it test that a schema update script is empty ?

Shouldn't we rather test that the schema update is correctly applied?

BTW I tried to revert the fix, and the test will still pass - so there's something probably not working as expected?

Copy link
Contributor Author

@dreab8 dreab8 Dec 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we rather test that the schema update is correctly?

depends what you want to test, the issue is the that the update tries to recreate existing tables so we have to check that the update script is empty.

I'll change the name of the test method and the code to express better the aim of the test

I tried to revert the fix, and the test will still pass

You have to try it with MySql

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we specify mysql dialect for the testing, then?

...est/java/org/hibernate/test/schemaupdate/SchemaUpdateTestWithUseJdbcMetadataDeaultsTest.java Outdated
.setFormat( false )
.execute( EnumSet.of( TargetType.DATABASE, TargetType.SCRIPT ), metadata );

final String fileContent = new String( Files.readAllBytes( output.toPath() ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should you use getAbsolutePath here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getAbsolutePath() returns a String while Files.readAllBytes requires a Path

NathanQingyangXu
NathanQingyangXu reviewed Dec 18, 2019
View reviewed changes
...est/java/org/hibernate/test/schemaupdate/SchemaUpdateTestWithUseJdbcMetadataDeaultsTest.java Outdated
public class SchemaUpdateTestWithUseJdbcMetadataDeaultsTest {

@Parameterized.Parameters
public static Collection<String> parameters() {
Copy link
Contributor

@NathanQingyangXu NathanQingyangXu Dec 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, it seems you can use String[] directly without the wrapping of Arrays.asList. See https://github.com/junit-team/junit4/wiki/Parameterized-tests#tests-with-single-parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank @NathanQingyangXu I applied your suggestion

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be me who say thank you. I borrowed the testing methodology in this PR and applied it in my PR: #3144. During the process, I found we can use array directly. Glad I can help.

@dreab8
Copy link
Contributor Author

dreab8 commented Dec 19, 2019

@Sanne thanks for the review, I pushed a commit with some changes to the test and any further suggestion to improve it is more than welcome.

@Sanne
Copy link
Member

Sanne commented Jan 7, 2020

hi @dreab8 , I'm back to trying this out and still have some issues.

When I run:

./gradlew :hibernate-core:test -Pdb=pgsql --tests SchemaUpdateWithUse*

The two new tests fail: org.hibernate.tool.schema.spi.SchemaManagementException: Schema-validation: missing table [TEST_ENTITY]

@dreab8
Copy link
Contributor Author

dreab8 commented Jan 8, 2020

Hi @Sanne, I checked and it seems the changes fix the issue for MySql but not for PostgreSQL. I'm investigating.

Thanks for testing with PostgreSQL.

Base automatically changed from master to main March 19, 2021 16:00
@yrodiere
Copy link
Member

yrodiere commented Jun 21, 2021 •
edited
Loading

Hey. I'm having a look at a few long-standing Quarkus bugs, and it seems this PR was supposed to fix quarkusio/quarkus#5883 .

@dreab8 did you find out what was the issue with PostgreSQL in the end? Do you think we could adapt the fix?

@Sanne
Copy link
Member

Sanne commented Jun 21, 2021

AFAIR this relates to the fact that we didn't have the right metadata - could possibly be solved now since we actually load it? Not sure if the timing is right in relation with schema gen.

But yes even if this is still not fixed, it might be easier to solve now.

@dreab8
Copy link
Contributor Author

dreab8 commented Jun 21, 2021

@yrodiere @Sanne I locally rebased my branch on main and run the SchemaUpdateWithUseJdbcMetadataDefaultsSettingToFalseTest test with PostgreSQL and it passed, I pushed force... let's see if all the tests pass on Github 🤞

@dreab8
Copy link
Contributor Author

dreab8 commented Jun 21, 2021 •
edited
Loading

the SchemaUpdateWithUseJdbcMetadataDefaultsSettingToFalseTest set hibernate.temp.use_jdbc_metadata_defaults to false so for the identifier the default MIXED case strategy is used (https://github.com/hibernate/hibernate-orm/blob/main/hibernate-core/src/main/java/org/hibernate/engine/jdbc/env/spi/IdentifierHelperBuilder.java#L41 ), so for databases using an UPPER startegy the schema validation process fails.

By the way https://github.com/hibernate/hibernate-orm/blob/main/hibernate-core/src/main/java/org/hibernate/engine/jdbc/env/spi/IdentifierHelperBuilder.java#L41 seem wrong to me

@dreab8
Copy link
Contributor Author

dreab8 commented Jun 21, 2021

@Sanne @yrodiere locally I have a fix for the default strategy issue, I need to test it a little more and not sure if it belongs to the same Jira or if we need to create a separate Jira for it

@dreab8 dreab8 force-pushed the HHH-13788 branch 6 times, most recently from 455fb0c to 226766b Compare June 22, 2021 13:33
@dreab8 dreab8 merged commit f1a5b2c into hibernate:main Jun 22, 2021
@Sanne Sanne mentioned this pull request Jun 22, 2021
Closed
@mpeddada1 mpeddada1 mentioned this pull request Feb 25, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NathanQingyangXu NathanQingyangXu NathanQingyangXu left review comments

@Sanne Sanne Sanne requested changes

@sebersole sebersole Awaiting requested review from sebersole

@gsmet gsmet Awaiting requested review from gsmet

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Hibernate hbm2ddl in application.properties is not working as mentioned
4 participants
@dreab8 @Sanne @yrodiere @NathanQingyangXu